Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix test CI issue by calling self test action #217

Merged
merged 24 commits into from
Mar 22, 2024
Merged

Fix test CI issue by calling self test action #217

merged 24 commits into from
Mar 22, 2024

Conversation

shenxianpeng
Copy link
Collaborator

closes #212

@shenxianpeng shenxianpeng changed the title Checkout pull request HEAD commit fix latest tag was added to the incorrect commit hash Mar 21, 2024
.github/workflows/run-test.yml Outdated Show resolved Hide resolved
@github-actions github-actions bot added the bug Something isn't working label Mar 21, 2024
2bndy5

This comment was marked as resolved.

@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Mar 21, 2024

We have push events disabled so we can't test merge commits, which is a bit of a concern for me.

but if we enable the push event, we will see the following comments in both the merge commit and the pull request here.

if we set thread-comments: false, we will miss a feature test. That is a problem.

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-format reports: 2 file(s) not formatted
clang-tidy reports: 6 concern(s)
Have any feedback or feature suggestions? Share it here.

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 21, 2024

We are seeing the thread-comment in this thread because the GITHUB_REPOSITORY env var is set to "cpp-linter/cpp-linter-action" (not "cpp-linter/test-cpp-linter-action"). There are other problems that we have been trying to workaround as well, all the problems relate to the fact: The cpp-linter package is not meant to be executed on a certain repo that is not same repo triggering the workflow.

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 21, 2024

The cpp-linter package is not meant to be executed on a certain repo that is not same repo triggering the workflow.

I don't think this is a feature anyone would want in cpp-linter because it seems specific to our use case.

I think the solution is to switch back to running cpp-linter on this repo. Instead of using docs/examples/demo folder, we could add the test repo as a submodule of this repo and set ignore to explicitly not ignore the submodule. With the submodule checked out, we could then call a reusable workflow from the submodule's path (I think).

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 21, 2024

With the submodule checked out, we could then call a reusable workflow from the submodule's path (I think).

This idea won't work exactly as stated. We can't checkout a submodule (in 1 job) then call the reusable workflow from the submodule's path (in another job).

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 21, 2024

image

Remember, the changes to this branch are not tested yet. The pull_request_target executes in the context of the target (aka base) branch, so all changes in this head branch are basically ignored.

@shenxianpeng
Copy link
Collaborator Author

The cpp-linter package is not meant to be executed on a certain repo that is not same repo triggering the workflow.

if possible to change GITHUB_REPOSITORY to cpp-linter/test-cpp-linter-action before running test action?

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 21, 2024

According to GH docs:

You can't overwrite the value of the default environment variables named GITHUB_* and RUNNER_*.

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 21, 2024

Docs about using on.push.tags
This seems more applicable to the triger that calls the reusable workflow:

on:
  push:
    tags:
      - latest

@shenxianpeng
Copy link
Collaborator Author

shenxianpeng commented Mar 22, 2024

It seems we don't need to call reusable workflow from cpp-linter/.github, we can use uses: ./ to self test action like this example

https://github.com/pre-commit/action/blob/main/.github/workflows/main.yml#L12-L13

with that, we may not need to create latest tag for testing

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 22, 2024

oh, I never thought to use ./. That's very clever.

@2bndy5
Copy link
Collaborator

2bndy5 commented Mar 22, 2024

The self test CI failed because

  1. It found errors in the C source files that installed with pygit2. Basically it didn't ignore the venv that it was running from.

  2. Body is too long (maximum is 65536 characters).

    We really need to review/test conditionally create comment cpp-linter#91 to avoid this "comment too long" error

@shenxianpeng shenxianpeng changed the title fix latest tag was added to the incorrect commit hash Fix test CI issue by calling self test action Mar 22, 2024
@shenxianpeng
Copy link
Collaborator Author

I ignored the venv path and the CI passed. How about now, ready to merge?

Copy link
Contributor

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-format reports: 2 file(s) not formatted
  • docs/examples/demo/demo.cpp
  • docs/examples/demo/demo.hpp
clang-tidy reports: 7 concern(s)

Have any feedback or feature suggestions? Share it here.

Copy link
Collaborator

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that you just copied most of the workflow from our test repo, But that workflow is a little neglected.

.github/workflows/self-test.yml Outdated Show resolved Hide resolved
.github/workflows/self-test.yml Outdated Show resolved Hide resolved
.github/workflows/self-test.yml Outdated Show resolved Hide resolved
@shenxianpeng shenxianpeng merged commit 7c32efe into main Mar 22, 2024
35 checks passed
@shenxianpeng shenxianpeng deleted the fix-212 branch March 22, 2024 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrectly added latest tag for PR testing and not trigger test action
2 participants